Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

MPDX-7893 - Allowing users to add offline organizations to their accounts #936

Merged
merged 2 commits into from
May 1, 2024
Merged

MPDX-7893 - Allowing users to add offline organizations to their accounts #936

merged 2 commits into from
May 1, 2024

Conversation

dr-bizz
Copy link
Contributor

@dr-bizz dr-bizz commented Apr 30, 2024 •
edited
Loading

Description

Fixing error introduced in #891

Currently, on Connect Services, users aren't able to add offline organizations due to the disableNewUsers returning as NULL from GraphQL.

The issue was caused by a FormIk client error. I've enabled disableNewUsers to be nullable and added a test to ensure NULL is treated the same way as TRUE.

Checklist:

  • I have given my PR a title with the format "MPDX-(JIRA#) (summary sentence max 80 chars)"
  • I have applied the appropriate labels. (Add the label "On Staging" to get the branch automatically merged into staging.)
  • I have requested a review from another person on the project

@dr-bizz dr-bizz requested review from caleballdrin and canac April 30, 2024 21:25
@github-actions GitHub Actions
Copy link
Contributor

Bundle sizes [mpdx-react]

Compared against 427565b

No significant changes found

caleballdrin
caleballdrin approved these changes Apr 30, 2024
View reviewed changes
Copy link
Contributor

@caleballdrin caleballdrin left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. thanks for fixing this and adding the test

@dr-bizz dr-bizz added On Staging Will be merged to the staging branch by Github Actions Preview Environment Add this label to create an Amplify Preview labels Apr 30, 2024
@github-actions GitHub Actions
Copy link
Contributor

Preview branch generated at https://fix-add-organization-modal.d3dytjb8adxkk5.amplifyapp.com

canac
canac approved these changes Apr 30, 2024
View reviewed changes
Copy link
Contributor

@canac canac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing all these bugs! I left an optional suggestion that I must have missed in previous code review.

src/components/Settings/integrations/Organization/Modals/OrganizationAddAccountModal.tsx Outdated
@@ -153,7 +153,7 @@ export const OrganizationAddAccountModal: React.FC<
name: yup.string().required(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to move this schema outside of the component? I don't think it uses anything from the component closure. Having it inside the component means it will be unnecessarily recreated on every render.

@dr-bizz dr-bizz merged commit 8753f06 into main May 1, 2024
18 checks passed
@dr-bizz dr-bizz deleted the fix-add-organization-modal branch May 1, 2024 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caleballdrin caleballdrin caleballdrin approved these changes

@canac canac canac approved these changes

Assignees
No one assigned
Labels
On Staging Will be merged to the staging branch by Github Actions Preview Environment Add this label to create an Amplify Preview
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dr-bizz @canac @caleballdrin